Repository navigation
ci: quarantine flaky tests against a ticket instead of retrying past them - #777
Merged
Merged
Conversation
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
CI Test ResultsRun: #36586982743 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-29 16:08:12 UTC |
Contributor
This comment has been minimized.
This comment has been minimized.
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
A normal musl test failure makes Gradle print the same text that the classifier treats as a dead test JVM. This makes every quarantined musl failure keep the CI job red.
🤖 Datadog Autotest · Commit 5f01c39 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f01c39cc0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…them CI could not tell a flaky test from a broken one, and its one retry erased the evidence either way. - The retry existed only for ASan. A test that failed on attempt 1 and passed on attempt 2 left a green job and a ::warning:: in a log, naming nothing. - Test reports upload only `if: failure()`, so the run that recovered on a retry -- the one worth studying -- produced no artifact at all. - generate-test-summary.sh already downloaded `(test-reports)*` and grepped TEST-*.xml for failed test names, but prepare_reports.sh copies build/reports/tests (the HTML) and never build/test-results (the XML), so it searched artifacts containing no XML and every failed job rendered "No detailed failure information available". A `**` glob with no `shopt -s globstar` would have stopped it recursing even had the files been there. Retrying until green would only have made the tolerance official. Instead the retry now buys a label and nothing else, and an explicit list decides what may fail: flaky failed one attempt, passed another broken failed every attempt gating not on the quarantine list -- red, whichever of the above it is So a flake fails the build until somebody quarantines it against a PROF ticket. ddprof-test/quarantine.txt is a plain "|"-separated table, one entry per line, chosen over JSON/YAML because it is edited by hand far more than by machine: real comments, one-line diffs, clean git blame, and no parser beyond str.split (it must also load inside the Alpine containers, where PyYAML is not a given). Every entry carries a ticket and a review_by date, and validate-quarantine fails CI once that date passes -- otherwise the list only grows and quarantine becomes a permanent mute rather than tracked debt. Quarantined tests still run and still report; only the gating is suspended, so the pass rate keeps saying whether the test is recovering or has quietly become permanently broken. To keep the honest path the cheap one, the PR comment prints a filled-in entry to paste, with a `cells` glob narrowed to the axis that actually failed. The ticket and the judgement stay with a person; the typing does not. Reporting is grouped by test rather than by cell -- one flaky test reddens a dozen cells and so do a dozen unrelated breakages -- and per-cell outcomes now upload whether the cell passed or failed, since a cell that failed only on its first attempt produces no failure artifact. Failing to classify is itself a failure: if flake_report.py cannot run, the job goes red rather than inheriting a pass nothing examined. An earlier draft had `|| true` there and turned a real flake green in testing. test_quarantine.sh covers the gating decisions against fixtures, including that an un-quarantined flake stays red, a quarantined one does not, a build error is never excused by the list, and an unreadable list cannot yield green. It runs in the validate-quarantine job. The retry path only executes once something has failed, so CI would otherwise never exercise it. Deferred: auto-filing PROF tickets (needs dedupe and an Atlassian credential for CI) and the GitLab dd-trace integration matrix, which still gets one shot per config.
Two defects that would have mattered: Quarantine excused too much. When every named test failure was on the list, the runner forced a green exit -- including when the same invocation had also failed a native or verification task, which the list has no business excusing. The runner now scans the attempt's log for `Execution failed for task` naming anything outside the test task and refuses to zero the exit code. The documented cell glob could never match. Cells are named <libc>-<jdk>-<config>-<arch> with arch amd64 or aarch64, so the `*arm64*` in quarantine.txt's example and in flake_summary.py's axis list matched nothing: the narrowing they advertised silently quarantined everywhere. Both use aarch64 now, and `validate` rejects a glob naming an architecture CI never builds. Also: - flaky now means failed once and observed passing on another attempt, not merely absent from it. An attempt that aborted early no longer turns every earlier failure into a flake with a paste-ready entry. - the runner clears its own evidence directory, so a reused workspace cannot contribute a previous run's attempts to this run's gating. - the counter and the gating read are checked rather than defaulted to zero, matching the fail-loud policy already applied to the classifier. - Docker-written results are made readable before snapshotting, and the snapshot warns instead of discarding errors; musl-aarch64 was losing flake classification silently. - pipes in failure messages are escaped, flaky tests appear in the per-job details, and an unparseable outcome report is visible rather than rendering as a clean non-test failure. - validating a missing list fails instead of reporting zero problems; duplicate detection keys on the cell globs, so narrowing by cell is actually usable. - one first-match helper shared by both selection paths. The regression test for the classifier guard did not exercise it -- a malformed line is skipped, not fatal, so the flake was the reason for the red. It now points --list at a directory to make the classifier genuinely fail. Each new guard was mutation-checked: reverting it turns the corresponding test red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The exit code the quarantine list overrides comes from the final attempt, but the failures it was weighed against were aggregated across all of them. A final attempt that failed without naming a test -- a docker failure in the musl-aarch64 job, a Gradle configuration error, an OOM-killed daemon, the ASan init abort this retry exists for -- was excused as soon as one entry matched a failure from an earlier attempt. flake_report.py now reports the final attempt's own standing and the runner refuses to zero the exit code unless that attempt produced results with every one of its own named failures quarantined; the non-test-task grep stays as a second line of defence rather than the only one. Alongside it: - validate rejects overlapping cell globs, not just byte-identical ones, and reports an out-of-range review_by as an annotated problem instead of an uncaught ValueError that loses every other annotation in the file - the dead `propose` subcommand goes; flake_summary.py already renders the paste-ready entry CI actually uses - test ids and failure messages are sanitised before they reach the PR comment, so a test's own output cannot break out of the fenced quarantine proposal a reviewer is invited to copy - a summary with no readable outcome reports says so rather than looking like a clean run - the cell label carries the slow/regular axis, so nightly's two invocations of the same config stop colliding in ci-outcome/<cell>.json - testcase elements with no name are skipped instead of being counted and proposed for quarantine as "." - make_results_readable probes per-file ownership rather than the top of the tree, and covers the parent so the pre-attempt rm -rf can unlink it - flake-evidence/ and ci-outcome/ are gitignored Tests: the clean-pass path and the final-attempt-named-no-test regression are now covered (27 assertions), and a new test_generate_test_summary.sh pins generate-test-summary.sh's jq failure branch -- verified by removing the `!` and watching it go red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A final attempt that crashes part-way through still writes JUnit XML for the tests it reached, and those passed -- so it names no failure of its own while every aggregated failure is quarantined. Gradle attributes the abort to the test task itself, so the non-test-task check cannot see it either. The final attempt's own exit code is the only thing that tells this apart from an ordinary flaky-then-passed run, so the runner now hands it to the classifier, which gates a non-zero exit that named nothing. Two evidence-integrity holes alongside it: - snapshot() takes read access again before copying. The XML is written by the command that just ran, after the loop-top call, and under Docker it lands root-owned; without this the copy fails and the cell loses flake classification silently. - make_results_readable() reports failure instead of warning and returning success, and its callers turn that into EVIDENCE_SUSPECT. A snapshot missing root-owned files is indistinguishable from an attempt whose missing tests all passed, which is exactly what the quarantine list must not be allowed to excuse. Three assertions cover these; each was mutation-checked individually. One of them asserts the ordinary quarantined-flake-recovers case stays green, so the new gate cannot be satisfied by reddening everything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
musl produced no JUnit XML, so every mechanism here read nothing for it: the retry never fired, no failure could be classified, and a passing attempt was indistinguishable from one that never ran. ProfilerTestRunner now registers a LegacyXmlReportGeneratingListener when -Dtest.reportsDir is set, and ProfilerTestPlugin points it at build/test-results/<taskName> -- the same layout Gradle's own Test task uses, so the TEST-*.xml scan finds musl's results exactly as it finds every other cell's. Adds junit-platform-reporting to the testing bundle. Four gating holes: - A non-zero final attempt gates regardless of how many of its own failures were named and quarantined. A JVM can abort part-way through *after* naming a real quarantined failure, leaving the tests it never reached absent from the XML rather than passing; requiring zero named failures let one quarantined name paper over the crash. - quarantine.py enforces review_by at match time, not only in the validate subcommand. validate-quarantine runs from ci.yml alone, while find_entry is reached from every workflow that reuses run_tests_with_retry.sh, so an expired entry could keep excusing failures in nightly and release-validated. - cells_glob listed its axes out of cell-name order. Cell names read <libc>-<jdk>-<config>-<arch>[-slow] and the glob is built by `*`-joining, so an axis order of aarch64 before musl produced `*aarch64*musl*` -- a proposal that fnmatch can never match against the cells it was derived from. A test now asserts every generated glob matches its own cells. - A failed snapshot sets EVIDENCE_SUSPECT rather than only warning, so results that could not be copied reach the classifier as suspect evidence instead of looking like an attempt whose missing tests all passed. All 35 quarantine tests and 3 summary tests pass; :ddprof-test:compileTestJava succeeds with the new dependency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A quarantined test that fails makes Gradle exit non-zero by itself, so gating
on any non-zero final-attempt exit code gated the ordinary case the list
exists to excuse. Only "failed on an earlier attempt, then passed outright on
the final one" could ever be excused, and on slow suites -- MAX_ATTEMPTS=1, so
no earlier attempt exists -- the list had no effect at all. That contradicts
quarantine.txt's first line.
A non-zero exit now gates only with positive evidence that the attempt was cut
short rather than run to completion:
- Gradle reporting a dead test JVM in the attempt log ("finished with non-zero
exit value"), or the JVM's own fatal-error banner
- the final attempt having reached fewer tests than another attempt managed,
which needs more than one attempt and so does not help slow suites
Both are reported in the outcome JSON. A missing log yields no evidence and
does not gate, matching how non_test_task_failures() already treats it.
No test passed --final-attempt-exit-code, so it defaulted to None, took the
(None, 0) exemption, and the whole guard went unexercised. Three fixtures now
cover it: a quarantined failure with exit 1 and an ordinary test-task failure
in the log must not gate; the same with a dead-JVM log must gate; and a final
attempt that reached fewer tests than an earlier one must gate. 38 assertions
pass, and each new guard was mutation-checked.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gradle records a JUnit 5 @test method as name="method()", so the id built from the report is Class.method(), while quarantine.txt documents -- and its own worked example uses -- Class.method. covers() compares by exact equality, so an entry written exactly as documented matched nothing: the failure still gated, validate still reported the list as valid, and the only symptom was a quarantine that quietly excused nothing. Confirmed against this repo's own reports, which carry both name="attributeOverflowReturnsFalse()" and name="[1]". normalise_test_id() strips the trailing parentheses, and covers() applies it. That keeps the normalisation in the one function every caller's match goes through, and leaves reports and annotations showing the id JUnit produced rather than a rewritten one. Two shapes can never match, so validate now rejects them instead of passing them as valid: - a test field carrying JUnit's parentheses, which the normalisation above makes unnecessary - one whose last segment is an invocation index. @ParameterizedTest and @RetryingTest invocations appear as "[1]", "[2]" with no method name at all, so the index identifies neither the method nor a stable case; the class-wide "<class>.*" form is the only thing that can cover them, and quarantine.txt now says so. The paste-ready proposal keeps a real method name for the first shape. sanitize_quarantine_test_pattern() widened anything containing punctuation to the whole class, and "()" is punctuation, so every per-method proposal came out as <class>.* -- muting far more than the evidence supported. The parens are now stripped before that test, while an indexed invocation still proposes the class, which is correct for it. 42 assertions pass. Each guard was mutation-checked: dropping the normalisation, either validation, or the precise proposal turns its own assertion red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every item here shares one shape: something the machinery could not read, or could not express, was treated as permission to proceed rather than as a reason to stop. Each now has a fixture, and each fixture was mutation-checked against its own guard. quarantine.py - An unreadable review_by (blank, or not a calendar date -- parse() accepts both) made an entry never expire. Since validate() only runs in PR CI while find_entry() is reached from nightly and release-validated too, the one entry nobody can review outlived every entry that could. It now counts as expired. - A trailing ".*" was an unbounded prefix match, so "com.datadoghq.profiler.*" suspended the merge gate for the whole repository from a single validate-clean line. It now covers one class's methods, as quarantine.txt has always claimed, and validate rejects a pattern that reads as a package. - A bare ".*" was validated against the empty string and accepted, then matched nothing. - The synthetic cell universe listed a non-existent 8-graal and omitted every -j9/-ibm/-orcl variant plus 21-graal and 25-graal. cells_overlap() fails closed for a glob matching nothing synthetic, so two genuinely disjoint entries (say *17-j9* and *21-graal*) were rejected as duplicates. The JDK axis is now the cross-product of the bases and the suffixes CI builds. flake_report.py - --evidence-suspect gated unconditionally, before establishing there was any failure to excuse: a suite that passed while its results tree could not be cleared was turned from exit 0 into a red job. Suspect evidence is a reason to distrust a quarantine excuse, so it now applies only where an excuse is being weighed. - A single attempt cannot distinguish flaky from broken -- passed_in is empty for every failure because nothing re-ran -- yet every slow-suite failure was reported as "persistent" and offered no quarantine entry. Those now land in an "unclassified" bucket which the summary renders with proposals and says plainly that flakiness was not measured. flake_summary.py - cells_glob() matched a hardcoded token list, omitting the JDK and the regular/slow axes -- the two the matrix varies along most. A flake seen only on glibc-8-j9-debug-amd64 proposed *glibc*debug*amd64*, quarantining it on all 13 JDKs and on the slow suite too. The axes are now read positionally out of the cell grammar, so an axis nobody thought of cannot be dropped. - A proposal widened to the whole class (the only form covers() can express for an invocation index) said nothing about it. It now carries a WIDENED line naming the id that could not be expressed exactly. run_tests_with_retry.sh - An empty command list degraded to a bare redirection, exited 0, recorded no attempt, and passed the cell without running a test. - A destination that could not be cleared left cp -r merging this attempt's evidence with an earlier one's, so attempt_results() reported tests as observed-and-passed that this attempt never ran -- which is how a persistent failure acquires a flaky label and a paste-ready entry that buries a real defect. Both that and the run-start clear now record suspect evidence instead of discarding the status. prepare_reports.sh - The fallback that ships build/test-results keyed off the attempt-* directory, which snapshot() creates before the copy that may fail. An empty or partial snapshot therefore uploaded an artifact with no JUnit XML in it. It now tests for actual XML, and has its own hermetic suite, wired into the validate job. ProfilerTestRunner - The musl XML path discarded mkdirs()'s result, and the listener reports write failures to a PrintWriter rather than throwing, so a musl run could finish with no TEST-*.xml and look like a cell with nothing to report. It now fails loudly when reports were requested and cannot be written. Also: the non-test-task guard was asserted on stdout the fixture echoed itself, so deleting it left the test green; it is now asserted on the recorded gate_reason with the fixture exiting 0 to isolate it from the exit-code guard. And the job-name regex is checked against test_workflow.yml's own name: expressions, so a rename fails in the validate job rather than silently rendering a summary that reports zero test jobs. 56 quarantine assertions, 4 summary, 2 prepare-reports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the quarantine scripts or their tests locally leaves .github/scripts/__pycache__/, which showed up as untracked in every worktree that had run them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Flagged by review against 5f01c39: musl runs ProfilerTestRunner through a plain Exec task (see ProfilerTestPlugin), not Gradle's native Test task. Gradle prints "Process '...' finished with non-zero exit value" whenever that Exec'd process exits non-zero for *any* reason, including an ordinary single assertion failure -- ProfilerTestRunner reports failure through its own exit code rather than staying alive the way a Test-task worker does. Matching that line marked every musl test failure "cut short", so a quarantined musl flake could never be excused and stayed permanently red. Splits _CUT_SHORT_RE into _CRASH_RE (the JVM's own crash banner, reliable on every platform) and _WORKER_DIED_RE (the Gradle message, reliable only for glibc/macOS's native Test task). cut_short_marker() takes musl=<bool>, derived from the cell's leading <libc> token (quarantine.KNOWN_LIBCS), and drops _WORKER_DIED_RE for musl cells. Two new test_quarantine.sh cases: the exact false positive (musl, ordinary failure, Exec-task exit line -> must not gate) and its guard (musl, genuine crash banner -> must still gate). All existing cases, including the glibc cut-short case this line still correctly gates, still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
force-pushed
the
ci/flake-visibility
branch
from
September 21, 2026 10:35
5f01c39 to
af95e15
Compare
Flagged by review: cells_glob() correctly returns None when cells can't share a computable glob (a JDK variant like "17-j9" gives its cell one more "-"-separated field than a plain "17" cell -- both occur in the real matrix for the same test). render_proposals() then did `cells_glob(...) or []`, and applies_to() documents an empty `cells` column as "no globs means everywhere" -- so the auto-generated paste-ready proposal for an ordinary two-cell flake could silently suggest quarantining the test on every libc, config, arch and the slow suite too, the exact blast radius "Proposals narrow on every axis" exists to prevent. Falls back to the literal sorted observed cells instead of []: they can only narrow what the entry matches, never widen it, so it's a safe fallback even though it isn't a glob. New test_quarantine.sh case: mixed-width cells (a plain and a -j9 cell for the same test) must render as those literal cells, not the "-" global-quarantine sentinel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed both P1 findings on the musl "cut short" false positive, and the P2 finding on
All 59 |
jbachorik
approved these changes
Sep 21, 2026
jbachorik
left a comment
jbachorik
left a comment
Collaborator
There was a problem hiding this comment.
Looks feasible, let's give it a try!
rkennke
added a commit
that referenced
this pull request
Sep 21, 2026
build:x64-debug-diag and functional:x64-el7-debug-diag did their job: they showed the socket hooks fire correctly on EL7 (send/recv/write/ read all called, PLT-patched libjvm.so/libnio.so/libnet.so), and that the real cause of NativeSocketEvent count: 0 is the sampler's rate limiter legitimately rejecting nearly every call because NativeSocketTestBase.doTcpTransfer()'s "the send buffer fills after ~32 iterations and blocks" assumption doesn't hold on this runner's network stack -- a real test bug, not a CI/EL7 problem. That will be quarantined separately now that #777 is merged. Drops the diagnostic-only -Dddprof.debugStaticLibstdcxx gate in ConfigurationPresets.kt and the now-unused TESTS_FILTER passthrough in functional-tests.sh along with the jobs that were the only callers of either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 21, 2026
rkennke
added a commit
that referenced
this pull request
Sep 22, 2026
Follow-up to #777, for wiring the GitLab EL7 functional job into quarantine: EL7's base-repo python3 is 3.6.8 (no SCL/newer-Python repo exists for Oracle Linux 7), and running the real scripts inside that image crashed twice: add_subparsers(dest="command", required=True) TypeError: __init__() got an unexpected keyword argument 'required' datetime.date.fromisoformat(review_by) AttributeError: type object 'datetime.date' has no attribute 'fromisoformat' Both need Python 3.7+. Drops required=True (checking args.command is None manually afterward) and replaces fromisoformat with strptime(s, "%Y-%m-%d").date() everywhere it was called. Verified end-to-end inside the real Oracle Linux 7 image: a quarantined failure on its own cell now goes green under python3 3.6.8, and the existing 59 test_quarantine.sh assertions (which run under whatever python3 is on the host, typically 3.9+) still pass unchanged. Also adds "el7" to KNOWN_LIBCS: the GitLab functional job's Oracle Linux 7 runtime is technically glibc but must not share the "glibc" cell token with the GitHub Actions Ubuntu matrix -- confirmed by a second smoke test that a quarantine entry scoped to el7-8-release-amd64 does not excuse the identical failure on glibc-8-release-amd64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 22, 2026
functional:x64-el7-jdk* previously just ran gradlew directly and let its raw exit code decide the job -- ddprof-test/quarantine.txt had no effect on it at all, unlike the GH Actions matrix (#777). Wraps the gradlew invocation in run_tests_with_retry.sh with cell "el7-<jdk>-<config>-amd64". MAX_ATTEMPTS defaults to 1 (no retry) here specifically: the default of 2 means a full second suite run, and this runner has already OOMKilled the container outright once at the current heap/concurrency (PR #805). Losing the flaky-vs-broken distinction is an acceptable trade for not doubling memory pressure on a runner already this tight; revisit once there's more headroom. Extends .functional_job's artifact paths with flake-evidence/, ci-outcome/ and build/logs/attempt.log so a quarantine decision on this job has evidence to show for it. Verified end-to-end inside the real Oracle Linux 7 image with a stub gradlew: a quarantined failure on its own cell now exits 0 through the full functional-tests.sh-shaped invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 22, 2026
Follow-up to #777, for wiring the GitLab EL7 functional job into quarantine: EL7's base-repo python3 is 3.6.8 (no SCL/newer-Python repo exists for Oracle Linux 7), and running the real scripts inside that image crashed twice: add_subparsers(dest="command", required=True) TypeError: __init__() got an unexpected keyword argument 'required' datetime.date.fromisoformat(review_by) AttributeError: type object 'datetime.date' has no attribute 'fromisoformat' Both need Python 3.7+. Drops required=True (checking args.command is None manually afterward) and replaces fromisoformat with strptime(s, "%Y-%m-%d").date() everywhere it was called. Verified end-to-end inside the real Oracle Linux 7 image: a quarantined failure on its own cell now goes green under python3 3.6.8, and the existing 59 test_quarantine.sh assertions (which run under whatever python3 is on the host, typically 3.9+) still pass unchanged. Also adds "el7" to KNOWN_LIBCS: the GitLab functional job's Oracle Linux 7 runtime is technically glibc but must not share the "glibc" cell token with the GitHub Actions Ubuntu matrix -- confirmed by a second smoke test that a quarantine entry scoped to el7-8-release-amd64 does not excuse the identical failure on glibc-8-release-amd64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 22, 2026
functional:x64-el7-jdk* previously just ran gradlew directly and let its raw exit code decide the job -- ddprof-test/quarantine.txt had no effect on it at all, unlike the GH Actions matrix (#777). Wraps the gradlew invocation in run_tests_with_retry.sh with cell "el7-<jdk>-<config>-amd64". MAX_ATTEMPTS defaults to 1 (no retry) here specifically: the default of 2 means a full second suite run, and this runner has already OOMKilled the container outright once at the current heap/concurrency (PR #805). Losing the flaky-vs-broken distinction is an acceptable trade for not doubling memory pressure on a runner already this tight; revisit once there's more headroom. Extends .functional_job's artifact paths with flake-evidence/, ci-outcome/ and build/logs/attempt.log so a quarantine decision on this job has evidence to show for it. Verified end-to-end inside the real Oracle Linux 7 image with a stub gradlew: a quarantined failure on its own cell now exits 0 through the full functional-tests.sh-shaped invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 22, 2026
build:x64-debug-diag and functional:x64-el7-debug-diag did their job: they showed the socket hooks fire correctly on EL7 (send/recv/write/ read all called, PLT-patched libjvm.so/libnio.so/libnet.so), and that the real cause of NativeSocketEvent count: 0 is the sampler's rate limiter legitimately rejecting nearly every call because NativeSocketTestBase.doTcpTransfer()'s "the send buffer fills after ~32 iterations and blocks" assumption doesn't hold on this runner's network stack -- a real test bug, not a CI/EL7 problem. That will be quarantined separately now that #777 is merged. Drops the diagnostic-only -Dddprof.debugStaticLibstdcxx gate in ConfigurationPresets.kt and the now-unused TESTS_FILTER passthrough in functional-tests.sh along with the jobs that were the only callers of either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 22, 2026
Follow-up to #777, for wiring the GitLab EL7 functional job into quarantine: EL7's base-repo python3 is 3.6.8 (no SCL/newer-Python repo exists for Oracle Linux 7), and running the real scripts inside that image crashed twice: add_subparsers(dest="command", required=True) TypeError: __init__() got an unexpected keyword argument 'required' datetime.date.fromisoformat(review_by) AttributeError: type object 'datetime.date' has no attribute 'fromisoformat' Both need Python 3.7+. Drops required=True (checking args.command is None manually afterward) and replaces fromisoformat with strptime(s, "%Y-%m-%d").date() everywhere it was called. Verified end-to-end inside the real Oracle Linux 7 image: a quarantined failure on its own cell now goes green under python3 3.6.8, and the existing 59 test_quarantine.sh assertions (which run under whatever python3 is on the host, typically 3.9+) still pass unchanged. Also adds "el7" to KNOWN_LIBCS: the GitLab functional job's Oracle Linux 7 runtime is technically glibc but must not share the "glibc" cell token with the GitHub Actions Ubuntu matrix -- confirmed by a second smoke test that a quarantine entry scoped to el7-8-release-amd64 does not excuse the identical failure on glibc-8-release-amd64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 22, 2026
functional:x64-el7-jdk* previously just ran gradlew directly and let its raw exit code decide the job -- ddprof-test/quarantine.txt had no effect on it at all, unlike the GH Actions matrix (#777). Wraps the gradlew invocation in run_tests_with_retry.sh with cell "el7-<jdk>-<config>-amd64". MAX_ATTEMPTS defaults to 1 (no retry) here specifically: the default of 2 means a full second suite run, and this runner has already OOMKilled the container outright once at the current heap/concurrency (PR #805). Losing the flaky-vs-broken distinction is an acceptable trade for not doubling memory pressure on a runner already this tight; revisit once there's more headroom. Extends .functional_job's artifact paths with flake-evidence/, ci-outcome/ and build/logs/attempt.log so a quarantine decision on this job has evidence to show for it. Verified end-to-end inside the real Oracle Linux 7 image with a stub gradlew: a quarantined failure on its own cell now exits 0 through the full functional-tests.sh-shaped invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 24, 2026
build:x64-debug-diag and functional:x64-el7-debug-diag did their job: they showed the socket hooks fire correctly on EL7 (send/recv/write/ read all called, PLT-patched libjvm.so/libnio.so/libnet.so), and that the real cause of NativeSocketEvent count: 0 is the sampler's rate limiter legitimately rejecting nearly every call because NativeSocketTestBase.doTcpTransfer()'s "the send buffer fills after ~32 iterations and blocks" assumption doesn't hold on this runner's network stack -- a real test bug, not a CI/EL7 problem. That will be quarantined separately now that #777 is merged. Drops the diagnostic-only -Dddprof.debugStaticLibstdcxx gate in ConfigurationPresets.kt and the now-unused TESTS_FILTER passthrough in functional-tests.sh along with the jobs that were the only callers of either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 24, 2026
Follow-up to #777, for wiring the GitLab EL7 functional job into quarantine: EL7's base-repo python3 is 3.6.8 (no SCL/newer-Python repo exists for Oracle Linux 7), and running the real scripts inside that image crashed twice: add_subparsers(dest="command", required=True) TypeError: __init__() got an unexpected keyword argument 'required' datetime.date.fromisoformat(review_by) AttributeError: type object 'datetime.date' has no attribute 'fromisoformat' Both need Python 3.7+. Drops required=True (checking args.command is None manually afterward) and replaces fromisoformat with strptime(s, "%Y-%m-%d").date() everywhere it was called. Verified end-to-end inside the real Oracle Linux 7 image: a quarantined failure on its own cell now goes green under python3 3.6.8, and the existing 59 test_quarantine.sh assertions (which run under whatever python3 is on the host, typically 3.9+) still pass unchanged. Also adds "el7" to KNOWN_LIBCS: the GitLab functional job's Oracle Linux 7 runtime is technically glibc but must not share the "glibc" cell token with the GitHub Actions Ubuntu matrix -- confirmed by a second smoke test that a quarantine entry scoped to el7-8-release-amd64 does not excuse the identical failure on glibc-8-release-amd64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 24, 2026
functional:x64-el7-jdk* previously just ran gradlew directly and let its raw exit code decide the job -- ddprof-test/quarantine.txt had no effect on it at all, unlike the GH Actions matrix (#777). Wraps the gradlew invocation in run_tests_with_retry.sh with cell "el7-<jdk>-<config>-amd64". MAX_ATTEMPTS defaults to 1 (no retry) here specifically: the default of 2 means a full second suite run, and this runner has already OOMKilled the container outright once at the current heap/concurrency (PR #805). Losing the flaky-vs-broken distinction is an acceptable trade for not doubling memory pressure on a runner already this tight; revisit once there's more headroom. Extends .functional_job's artifact paths with flake-evidence/, ci-outcome/ and build/logs/attempt.log so a quarantine decision on this job has evidence to show for it. Verified end-to-end inside the real Oracle Linux 7 image with a stub gradlew: a quarantined failure on its own cell now exits 0 through the full functional-tests.sh-shaped invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 24, 2026
…#805) * ci: restore EL7 stresstest coverage, add nightly EL7 functional tests PR #790 moved build:x64 onto AlmaLinux 8, which took stresstest:x64 with it since it ran in the same image — runtime coverage at the shipped binary's glibc 2.17 floor went to zero. This follows up on the two items proposed there: - .stresstest_job now takes RUNTIME_IMAGE independently of BUILD_IMAGE, and stresstest:x64 points it at a new Oracle Linux 7 image so it goes back to running against glibc 2.17 while build:x64 stays on EL8. - adds a nightly functional:x64-el7-jdk{8,17,21} job matrix that runs the ddprof-test debug suite against the shipped x64 artifact on the same EL7 image, covering the libc-sensitive suites without multiplying the per-PR matrix. The new .gitlab/base/el7/Dockerfile is runtime-only (no compiler): both jobs consume the AlmaLinux-8-built .so via -Pskip-native -Pwith-libs. functional-tests.sh fetches the JDK under test from Adoptium into JAVA_TEST_HOME so Gradle itself keeps running on JDK 21 regardless of which JDK is under test. BUILD_IMAGE_X64_EL7 is a placeholder pin — the image still needs to be built and pushed via a real pipeline (REBUILD_IMAGES="x64-el7") before this can merge, same as #790's own image rebuild. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: run functional:x64-el7-jdk8 on every PR, not just nightly Keeps the 3-JDK sweep nightly but lets one JDK's worth of EL7 functional coverage run on every pipeline, so it isn't gated entirely on the nightly schedule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: pin BUILD_IMAGE_X64_EL7 to the rebuilt image digest Built and pushed successfully in pipeline 138399627, job 2056785023. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): install gcc-c++ in the EL7 image to satisfy Gradle's compiler probe stresstest:x64 failed with "No C++ compiler found": ConfigurationPresets. setupStandardConfigurations() calls PlatformUtils.findCompiler() unconditionally at project-configuration time, before -Pskip-native is consulted, so an image with no compiler at all fails to even configure the build. The stock EL7 gcc-c++ (4.8.5, base repo, no EPEL) satisfies that --version probe; -Pskip-native still gates every real compile/link task onlyIf, so it's never invoked to build anything. Verified the image still builds and g++ --version succeeds. This needs another `REBUILD_IMAGES="x64-el7"` run and a re-pin of BUILD_IMAGE_X64_EL7 before the fix takes effect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: repin BUILD_IMAGE_X64_EL7 to the gcc-c++ fixed image Rebuilt in pipeline 138438605 with the gcc-c++ fix from the previous commit; pushed and signed successfully before the rebuild-images log cuts off (mid-way through the unrelated datadog-ci image build). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): make functional-tests.sh executable functional:x64-el7-jdk8 failed with "Permission denied" — the script was committed as mode 100644 instead of 100755. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): raise test heap for the EL7 functional job ContendedCallTraceStorageTest OOM'd with "GC overhead limit exceeded" on jdk8: it drives availableProcessors()*2 threads of CPU/allocation load against the standard -Xmx512m test heap, and the GitLab shared runner pod apparently gives it less headroom than GitHub's ubuntu-latest runners do. Adds an opt-in -PtestMaxHeap gradle property to ddprof-test (appended after the plugin's default -Xmx512m, so the last -Xmx wins) and passes -PtestMaxHeap=1536m from functional-tests.sh only. The shared default used by every other caller (GH Actions matrix, stresstest, etc.) is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): let -Pskip-native target specific projects Several ddprof-test suites (NativeThread, DynamicNativeThread, ThreadEntryDetectionTest, ...) need ddprof-test-native's small JNI test helper on java.library.path regardless of which profiler binary is under test. Bare -Pskip-native disabled ALL native compilation project-wide, so those suites failed with UnsatisfiedLinkError/ NoClassDefFoundError on the EL7 functional job, which needs to skip only ddprof-lib's own build (substituted via -Pwith-libs) while still compiling that helper for real. PlatformUtils.isNativeSkipped() now supports -Pskip-native=<comma list of project names>, skipping only those; bare -Pskip-native (no value) keeps skipping everywhere, unchanged for every existing caller (stresstests.sh, build.sh, GH Actions). SimpleNativeLibPlugin (used by ddprof-test-native and malloc-shim) and ddprof-lib/build.gradle.kts's own skip-native check now go through it. functional-tests.sh passes -Pskip-native=ddprof-lib,malloc-shim (malloc-shim excluded too: it's unrelated to ddprof-test and its -std=c++17 flag doesn't compile under EL7's stock gcc 4.8.5). Verified locally: with -Pskip-native=ddprof-lib,malloc-shim, ddprof-test-native:linkLib actually compiles and links, while ddprof-lib:assembleDebugJar only runs copyExternalLibs (no native compile task in the graph at all). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): compile ddprof-test-native as C99 on Linux Now that -Pskip-native=ddprof-lib,malloc-shim actually compiles ddprof-test-native for the EL7 functional job, nativealloc.c failed: error: 'for' loop initial declarations are only allowed in C99 mode EL7's stock gcc (4.8.5) still defaults to gnu89; every other Linux gcc/ clang this project builds on already defaults to a C99+ dialect, so this was latent until a compiler this old actually built the file. Adds an explicit -std=gnu99. Verified against the real EL7 toolchain (oraclelinux:7 + gcc 4.8.5): all three ddprof-test-native source files compile and link. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): run testRelease, not testDebug, in the EL7 functional job build:x64 (build.sh) only ever produces and ships the release-config libjavaProfiler.so -- that IS the shipped artifact this job exists to exercise. Running testDebug set ddprof_test.config=debug while the loaded library was actually release-built, so debug-only assertions failed outright instead of assumeTrue-skipping correctly, e.g. JVMAccessTest.agentOnLoadVMFlagDetectionTest checking for [TEST::INFO] log lines that only exist in debug builds ("initLibrary not found", "DebugNonSafepoints flag lookup log line was not observed"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: add temporary diagnostic jobs for the NativeSocket* investigation functional:x64-el7-jdk8 has NativeSocketEvent count: 0 across every retry (zero, not just fewer-than-expected), suggesting the send/recv/ write/read hooks (libraryPatcher_linux.cpp's GOT patching) never actually get installed in this environment -- but that can't be told apart from "installed but never firing" without the debug-only hook-fire counters, which only exist in a debug-config build. build:x64-debug-diag builds a real debug-config libjavaProfiler.so on the same AlmaLinux 8 toolchain build:x64 uses (release configs don't carry NativeSocketSampler's #ifdef DEBUG counters/TEST_LOG calls). functional:x64-el7-debug-diag runs just the NativeSocket* suite against it on the EL7 runtime (via functional-tests.sh's new TESTS_FILTER -> -Ptests passthrough), so --info output shows whether send_hook/recv_hook/write_hook/read_hook ever get called at all. Both are marked TEMPORARY and should be removed once this is root-caused. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci(diag): statically link libstdc++ into the debug diagnostic build build:x64-debug-diag's first attempt failed to load on EL7: UnsatisfiedLinkError: libstdc++.so.6: version GLIBCXX_3.4.20 not found Debug configs link libstdc++ dynamically (only release gets -static-libstdc++), so the .so needed a newer GLIBCXX than EL7's stock gcc 4.8.5 provides. Statically linking (gated behind -Dddprof.debugStaticLibstdcxx=true, diagnostic-only) then surfaced a second, unrelated issue: -static-libstdc++ transitively pulls in libstdc++.a's cow-string-inst.o (legacy ABI string instantiations), which references std::random_device::_M_init -> getentropy@GLIBC_2.25 -- traced via the linker map (-Wl,-Map). That's dead code configureRelease's -Wl,--gc-sections already strips; debug's linkerArgs never had that flag. Adding --gc-sections and --exclude-libs,ALL alongside the static libstdc++ flags (same diagnostic gate) fixes both. Verified end to end: rebuilt in the real AlmaLinux 8 image, confirmed via objdump that the result has no libstdc++.so.6 NEEDED entry, no getentropy reference, and a max GLIBC_2.17 requirement, then loaded it inside the real EL7 image and saw VM::initLibrary log successfully. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: remove the temporary NativeSocket* diagnostic jobs build:x64-debug-diag and functional:x64-el7-debug-diag did their job: they showed the socket hooks fire correctly on EL7 (send/recv/write/ read all called, PLT-patched libjvm.so/libnio.so/libnet.so), and that the real cause of NativeSocketEvent count: 0 is the sampler's rate limiter legitimately rejecting nearly every call because NativeSocketTestBase.doTcpTransfer()'s "the send buffer fills after ~32 iterations and blocks" assumption doesn't hold on this runner's network stack -- a real test bug, not a CI/EL7 problem. That will be quarantined separately now that #777 is merged. Drops the diagnostic-only -Dddprof.debugStaticLibstdcxx gate in ConfigurationPresets.kt and the now-unused TESTS_FILTER passthrough in functional-tests.sh along with the jobs that were the only callers of either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: comments describe current state, not what changed "(rebuilt with gcc-c++ added, see el7/Dockerfile)" and "restoring the runtime coverage build:x64 lost when it moved to AlmaLinux 8 (PR #790)" tell a reader what changed relative to old code, not why the current code is the way it is -- and the -std=gnu99 comment named the exact source construct that motivated it, which goes stale the moment that code changes. State the current rationale only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: pre-review cleanup - comment history archeology, skip-native consistency - .gitlab-ci.yml: drop "unchanged from before" -- describes the diff against the file's prior state, not the current mechanism. - ddprof-lib/build.gradle.kts: its own skip-native check still used bare hasProperty("skip-native"), not PlatformUtils.isNativeSkipped(). Both agree today (every caller either omits the property, uses it bare, or lists "ddprof-lib" whenever it's meant to be skipped), so this isn't a behavior change -- confirmed locally: assembleDebugJar still runs only copyExternalLibs under -Pskip-native=ddprof-lib,malloc-shim, and still depends on the real compile/link without it. But a future -Pskip-native=<list not containing ddprof-lib> would silently skip ddprof-lib's own build too under the old bare check, contradicting isNativeSkipped()'s own doc comment. Fixes the one call site the new scoped semantics didn't reach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: cache the JDK-under-test tarball across functional job runs Flagged in review: functional-tests.sh's .jdk-cache/ was never in any cache paths -- .cache-config/.cache-config-pull only cover .gradle/caches/, .gradle/wrapper/, .m2/repository/ -- so every run re-downloaded the JDK tarball from Adoptium. Gives .functional_job its own two-entry cache instead of extending .cache-config-pull: the JDK tarball's natural cache key is the major version (TEST_JDK), not the branch/commit the Gradle cache is keyed on, and it doesn't change per-branch the way the Gradle cache's gradle-wrapper.properties hash does, so it gets pull-push with a static, ref-independent key while the Gradle entry stays pull-only exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: use isNativeSkipped() everywhere skip-native is checked Flagged in review: only ddprof-lib/build.gradle.kts and SimpleNativeLibPlugin.kt went through PlatformUtils.isNativeSkipped() after the earlier fix; GtestPlugin.kt, GtestTaskBuilder.kt, FuzzTargetsPlugin.kt and ddprof-lib/benchmarks/build.gradle.kts still used the bare hasProperty("skip-native") check, so -Pskip-native=<list not containing ddprof-lib> would silently skip gtest/fuzz/benchmarks too, contradicting the scoped semantics everywhere else now documents. GtestPlugin.kt/GtestTaskBuilder.kt are applied directly to ddprof-lib (project.name == "ddprof-lib" there already), so those are a direct swap. FuzzTargetsPlugin.kt and benchmarks/build.gradle.kts are applied to the :ddprof-lib:fuzz and :ddprof-lib:benchmarks *subprojects* (project.name == "fuzz"/"benchmarks") -- fuzzing and benchmarking are part of ddprof-lib's native surface, not something a caller names on its own, so those go through project.parent's identity instead of their own. Verified locally: -Pskip-native=ddprof-lib,malloc-shim still skips :ddprof-lib:benchmarks:compileBenchmark (SKIPPED, not executed), and it still compiles normally without the flag -- matching the old bare check's behavior for every current caller, while now correctly generalizing for a caller that doesn't name ddprof-lib. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 24, 2026
Follow-up to #777, for wiring the GitLab EL7 functional job into quarantine: EL7's base-repo python3 is 3.6.8 (no SCL/newer-Python repo exists for Oracle Linux 7), and running the real scripts inside that image crashed twice: add_subparsers(dest="command", required=True) TypeError: __init__() got an unexpected keyword argument 'required' datetime.date.fromisoformat(review_by) AttributeError: type object 'datetime.date' has no attribute 'fromisoformat' Both need Python 3.7+. Drops required=True (checking args.command is None manually afterward) and replaces fromisoformat with strptime(s, "%Y-%m-%d").date() everywhere it was called. Verified end-to-end inside the real Oracle Linux 7 image: a quarantined failure on its own cell now goes green under python3 3.6.8, and the existing 59 test_quarantine.sh assertions (which run under whatever python3 is on the host, typically 3.9+) still pass unchanged. Also adds "el7" to KNOWN_LIBCS: the GitLab functional job's Oracle Linux 7 runtime is technically glibc but must not share the "glibc" cell token with the GitHub Actions Ubuntu matrix -- confirmed by a second smoke test that a quarantine entry scoped to el7-8-release-amd64 does not excuse the identical failure on glibc-8-release-amd64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 24, 2026
functional:x64-el7-jdk* previously just ran gradlew directly and let its raw exit code decide the job -- ddprof-test/quarantine.txt had no effect on it at all, unlike the GH Actions matrix (#777). Wraps the gradlew invocation in run_tests_with_retry.sh with cell "el7-<jdk>-<config>-amd64". MAX_ATTEMPTS defaults to 1 (no retry) here specifically: the default of 2 means a full second suite run, and this runner has already OOMKilled the container outright once at the current heap/concurrency (PR #805). Losing the flaky-vs-broken distinction is an acceptable trade for not doubling memory pressure on a runner already this tight; revisit once there's more headroom. Extends .functional_job's artifact paths with flake-evidence/, ci-outcome/ and build/logs/attempt.log so a quarantine decision on this job has evidence to show for it. Verified end-to-end inside the real Oracle Linux 7 image with a stub gradlew: a quarantined failure on its own cell now exits 0 through the full functional-tests.sh-shaped invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rkennke
added a commit
that referenced
this pull request
Sep 24, 2026
* fix(ci): make the quarantine machinery run under Python 3.6 Follow-up to #777, for wiring the GitLab EL7 functional job into quarantine: EL7's base-repo python3 is 3.6.8 (no SCL/newer-Python repo exists for Oracle Linux 7), and running the real scripts inside that image crashed twice: add_subparsers(dest="command", required=True) TypeError: __init__() got an unexpected keyword argument 'required' datetime.date.fromisoformat(review_by) AttributeError: type object 'datetime.date' has no attribute 'fromisoformat' Both need Python 3.7+. Drops required=True (checking args.command is None manually afterward) and replaces fromisoformat with strptime(s, "%Y-%m-%d").date() everywhere it was called. Verified end-to-end inside the real Oracle Linux 7 image: a quarantined failure on its own cell now goes green under python3 3.6.8, and the existing 59 test_quarantine.sh assertions (which run under whatever python3 is on the host, typically 3.9+) still pass unchanged. Also adds "el7" to KNOWN_LIBCS: the GitLab functional job's Oracle Linux 7 runtime is technically glibc but must not share the "glibc" cell token with the GitHub Actions Ubuntu matrix -- confirmed by a second smoke test that a quarantine entry scoped to el7-8-release-amd64 does not excuse the identical failure on glibc-8-release-amd64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: add python3 to the EL7 image for the quarantine classifier functional-tests.sh is about to wrap its gradlew invocation in run_tests_with_retry.sh, which shells out to quarantine.py and flake_report.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: wire functional-tests.sh into the quarantine machinery functional:x64-el7-jdk* previously just ran gradlew directly and let its raw exit code decide the job -- ddprof-test/quarantine.txt had no effect on it at all, unlike the GH Actions matrix (#777). Wraps the gradlew invocation in run_tests_with_retry.sh with cell "el7-<jdk>-<config>-amd64". MAX_ATTEMPTS defaults to 1 (no retry) here specifically: the default of 2 means a full second suite run, and this runner has already OOMKilled the container outright once at the current heap/concurrency (PR #805). Losing the flaky-vs-broken distinction is an acceptable trade for not doubling memory pressure on a runner already this tight; revisit once there's more headroom. Extends .functional_job's artifact paths with flake-evidence/, ci-outcome/ and build/logs/attempt.log so a quarantine decision on this job has evidence to show for it. Verified end-to-end inside the real Oracle Linux 7 image with a stub gradlew: a quarantined failure on its own cell now exits 0 through the full functional-tests.sh-shaped invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: quarantine the NativeSocket* flake on el7-8-release-amd64 PROF-16014. Root-caused in PR #805: NativeSocketSampler's hooks install and fire correctly (confirmed via a debug-config diagnostic build), but the rate limiter rejects nearly every sample because NativeSocketTestBase.doTcpTransfer()'s assumption that the send buffer fills and blocks after ~32 iterations does not hold on this runner's network stack. Scoped to el7-8-release-amd64, the only cell this has actually been observed on -- not el7-*-release-amd64, so the nightly el7-17/-21 cells still gate for real if this doesn't reproduce there the same way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: keep the quarantine.txt example even with real entries present The example line was written to be deleted once the first real entry landed, but it's worth keeping as a permanent format reference -- the NativeSocket entries are tied to a bug that will get fixed and removed, and the file shouldn't go back to having no example once that happens. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * style: move _parse_date next to its first use Inserting it in the module-level constants block left it wedged between two densely-packed constant groups with no blank-line separation, breaking the file's own two-blank-lines-around-defs convention. Moves it next to is_expired(), its first caller, matching how the file already places its other small helpers near their use. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: quarantine NativeSocket* by class, not by exact method Flagged in review, and confirmed against both the test source and quarantine.txt's own documented format: every affected class's single test method is @RetryingTest, which -- like @ParameterizedTest and @testtemplate -- reports in JUnit XML as "Class.[1]", "[2]", ... with no method name at all. The exact-method entries could never match anything; only the already-class-wide NativeSocketStackTraceTest.* entry ever worked. Converts the other 8 to <class>.*. Also drops the NativeSocketUdpExcludedTest entry entirely rather than widening it: that test asserts *zero* events, which the bug's own symptom (near-zero events everywhere) would make pass, not fail. Its earlier observed failure has an unconfirmed, likely different cause, and quarantining it on this evidence risks hiding an unrelated regression later. Verified: find_entry() now matches the real "NativeSocketEnabledTest.[1]"-shaped id against the class-wide entry, and confirms the UDP test is not quarantined. All 59 test_quarantine.sh assertions and quarantine.py validate still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: repin BUILD_IMAGE_X64_EL7 to the python3-enabled image Built and pushed successfully in pipeline 139300629 (the job's overall failure was two unrelated images, arm64-musl and benchmarks-amd64). Confirmed python3 3.6.8 installed in the build log. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: raise container memory limit for EL7 functional job The x64-el7 functional job was getting OOMKilled (exit 137) at the container level during pod cleanup, after the test suite had already finished and uploaded its artifacts. The JVM under test already has its own -Xmx1536m cap; the pod itself needed more headroom to run Gradle's daemon/workers, the gtest builds, and the JVM together. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * build: compile build-logic in-process instead of a Kotlin daemon The Kotlin compile daemon outlives the build-logic compile and stays resident for the rest of the build, including the whole test run, next to the Gradle process and the test JVMs. Measured on a --no-daemon build: ~580MB Gradle + ~660MB Kotlin daemon with the default strategy, versus ~920MB Gradle and no daemon in-process. The out-of-process strategy was removed in Kotlin 2.4.0. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Revert "build: compile build-logic in-process instead of a Kotlin daemon" This reverts commit 7864e74. * ci: compile build-logic in-process only in the EL7 functional job The Kotlin compile daemon (~660MB RSS) stays resident through the whole test run in this 6GB pod. Setting in-process globally via build-logic/gradle.properties broke the CodeQL Java job: its Kotlin extractor runs inside the compiler, and in-process that exhausted Gradle's default 512m heap. Scope the setting to this job instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * Revert "ci: raise container memory limit for EL7 functional job" This reverts commit a794f69. * fix: gate a quarantined failure when the final attempt was killed by a signal A signal that kills the Gradle JVM itself (e.g. the OOM killer taking the --no-daemon build process) leaves nothing alive to print a cut-short marker, and with MAX_ATTEMPTS=1 there is no earlier attempt to measure a shortfall against. If a quarantined test had already written its failure XML, the report excused the run and the wrapper turned exit 137 into 0. Treat an exit status above 128 as the attempt being cut short. Gradle never exits above 128 on its own, however many tests fail. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix: honor skip-native=true as a global native skip gradle.properties.template documents skip-native=true, but isNativeSkipped() only treated an empty value as the global skip and read anything else as a list of project names. "true" matched no project, so native compilation ran anyway. Treat "true" like a bare -Pskip-native, and "false" as skipping nothing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Makes flaky tests a tracked, expiring debt instead of something CI quietly retries past.
A retry now buys a label, not a pass:
A flake fails the build until it is quarantined against a PROF ticket in
ddprof-test/quarantine.txt. Entries carry areview_bydate and expire: once it passes, the entry stops excusing anything and CI goes red again. Quarantined tests still run and report — only the gating is suspended, so the pass rate still says whether a test is recovering or has quietly become permanently broken. The PR comment prints a paste-ready entry for each flake, withcellsnarrowed to the axes that actually failed.Only the final attempt's own named failures are weighed against the list, and
flakyrequires observing a pass rather than mere absence from a later attempt. The list cannot excuse:Conversely, suspect evidence with no failure recorded holds no opinion: it is a reason to distrust an excuse, not a test failure, so a suite that passed stays green.
Motivation:
CI could not tell a flaky test from a broken one, and its single retry erased the evidence either way:
::warning::naming nothingif: failure(), so the recovered run — the one worth studying — produced no artifactgenerate-test-summary.shgreppedTEST-*.xmlout of artifacts that only ever heldbuild/reports/tests(HTML), neverbuild/test-results(XML), so every failed job rendered "No detailed failure information available"Answering "is this flaky?" therefore meant hand-comparing PRs by eye. Retrying until green would only have made the tolerance official.
Additional Notes:
musl coverage.
ProfilerTestRunnerruns musl's tests through the JUnit launcher directly and registered no XML listener. It now attaches aLegacyXmlReportGeneratingListenerwhen-Dtest.reportsDiris set, andProfilerTestPluginpoints that atbuild/test-results/<taskName>— the layout Gradle's ownTesttask uses — so theTEST-*.xmlscan finds musl's results the same way it finds every other cell's. It fails loudly if reports were requested and cannot be written, since producing none silently is indistinguishable from a cell with nothing to report.Patterns match the ids JUnit writes. Gradle records a
@Testmethod asname="method()", so ids arrive asClass.method()while entries are writtenClass.method;covers()normalises that.@ParameterizedTest/@RetryingTestinvocations appear as[1],[2]with no method name at all, so they can only be quarantined class-wide. A trailing.*now covers one class's methods, as the format has always documented, rather than being an unbounded prefix — so one validate-clean line can no longer suspend the merge gate repo-wide.validaterejects all three unmatchable shapes, and a proposal widened to the class says so with aWIDENEDline.Expiry is enforced where matching happens, not only in the
validatesubcommand:validate-quarantineruns fromci.ymlalone, whilefind_entryis reached from every workflow reusingrun_tests_with_retry.sh. An unreadablereview_bycounts as expired rather than as never expiring.Proposals narrow on every axis.
cells_globreads the shared axes positionally out of the<libc>-<jdk>-<config>-<arch>[-slow]grammar rather than matching a token list, so the JDK and the regular/slow axes cannot be silently dropped — a flake seen on one cell no longer proposes muting it across 13 JDKs and the slow suite.Behaviour changes. Retry now covers all four platform jobs, not just ASan, so a flake-shaped failure costs that cell a second run; slow suites are capped at one attempt to stay clear of the 180-minute timeout; ASan keeps retry-on-abort via
RETRY_ON_NO_TEST_FAILURES, naming no test when it dies at init.Status check names change. The four matrix jobs gain an explicit
name:, so their checks go fromtest-linux-glibc-amd64 (17, debug)totest-linux-glibc-amd64 (17, debug, regular). The suffix is required: this workflow is called twice in one run (nightly, release-validated) with overlapping configs, and without it two different jobs collapse onto the same cell. No ruleset on this repo requires status checks by name, so nothing should block on the old strings — but anything outside GitHub that matches on them would need the new form.How to test the change?:
Three hermetic suites, all run by the new
validate-quarantinejob:test_quarantine.sh(56 assertions),test_generate_test_summary.sh(4),test_prepare_reports.sh(2). Between them they cover every validation rejection, glob matching in both directions (a proposal matches the cells it came from and excludes those differing only in JDK or in the slow suffix), each thing the list cannot excuse, the classification edges (absent-from-retry, single-attempt, unnamed testcase), an empty command list, an artifact still shipping XML when a snapshot came out empty, and the rendered output. The summary suite also checks the job-name regex againsttest_workflow.yml's ownname:lines, so a rename fails here rather than rendering a summary that reports zero test jobs.Every guard was mutation-checked: reverting it individually turns its own assertion red.
The retry and gating paths only run once something has failed, so nothing in CI has exercised them end-to-end — they are covered by fixtures instead. Worth knowing when reading the risk of this PR.
Known gaps, deliberately not in this PR:
ProfilerTestRunnerto confirm the shape matches whatflake_report.pyparses. A mismatch there makes the whole list a no-op while every assertion stays green — which is exactly what theClass.method()bug above was. The highest-value follow-up.python3 -cand a two-line stdout protocol, rather thanflake_report.pycommunicating its verdict directly.For Datadog employees: